Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow using username and email in SMTP configuration #3398

Merged
merged 18 commits into from
Oct 21, 2024
Merged

Allow using username and email in SMTP configuration #3398

merged 18 commits into from
Oct 21, 2024

Conversation

samaradel
Copy link
Contributor

@samaradel samaradel commented Sep 10, 2024 •
edited
Loading

Description

  • Remove isDiscourse check and apply adding email or username for all apps with SMTP.
  • Add the minimum and maximum lengths for the username and email.
  • validate the email input to accept the username too.
  • Accept usernames containing dashes and underscores they must start with an alphanumeric character and validate them.

image

  • limit the password length to 69 chars as per sendgrid documentation
    image

Changes

image

Related Issues

Tested Scenarios

  • Check if the user used an email or username in the Email/username input and validate it.
  • Check if the input accepts less than 2 characters.
  • Check if the input accepts more than 50 characters.
  • Check if the username or email can start with anything except an alphanumeric character.
  • Check if we could leave the input empty

image

Documentation PR

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

@samaradel samaradel changed the title Remove isDiscourse check and apply adding email or username for all a… Allow username or email in SMTP configuration Sep 10, 2024
@samaradel samaradel changed the title Allow username or email in SMTP configuration Allow using username and email in SMTP configuration Sep 10, 2024
@samaradel samaradel marked this pull request as draft September 11, 2024 12:36
@samaradel samaradel marked this pull request as ready for review September 19, 2024 08:29
@samaradel @github-advanced-security
Apply code scanning fix for overly permissive regular expression range
bc4c149 
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@0oM4R
Copy link
Contributor

0oM4R commented Sep 22, 2024

please mention the idea behind the validation rules, 69 characters, and the regex of the username

xmonader
xmonader reviewed Sep 29, 2024
View reviewed changes
packages/playground/tests/utils/isValidSmtp.test.ts Outdated
});

it("returns an error message for white spaces in username", () => {
const input = "invalid username";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there're more types of spaces beyond " " there's is \t and \v

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted in the current validation.

@samaradel samaradel marked this pull request as draft October 1, 2024 08:08
@samaradel
- Add min length and max length for the username and Email
788077b 
- Accept usernames that contain dashs and underscores and it must start with an alphabetical character and validate them
@samaradel samaradel marked this pull request as ready for review October 2, 2024 10:25
@samaradel samaradel marked this pull request as draft October 2, 2024 11:58
@samaradel samaradel marked this pull request as ready for review October 2, 2024 12:52
0oM4R
0oM4R reviewed Oct 8, 2024
View reviewed changes
packages/playground/src/components/smtp_server.vue Outdated
return isDiscourse ? undefined : validators.isEmail('Please provide a valid email address.')(v);
validators.required('Email or Username is required.'),
validators.minLength('Username must be at least 2 characters.', 2),
validators.maxLength('Username must be at least 50 characters.', 50),
Copy link
Contributor

@0oM4R 0oM4R Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't use the at least on max length

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also is this validation on user name only? what if the email exceeds the 50 char ? the length validation message will appear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

0oM4R
0oM4R requested changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the validation need a revisit
Screenshot from 2024-10-08 11-13-30
Screenshot from 2024-10-08 11-13-39

@samaradel samaradel marked this pull request as draft October 9, 2024 07:56
@samaradel samaradel marked this pull request as ready for review October 9, 2024 10:37
@samaradel samaradel requested a review from 0oM4R October 9, 2024 10:37
0oM4R
0oM4R requested changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Check if the username or email can start with anything except an alphabetical character.

email pass the validation starting with number, also please add this test case to unit test
image

@0oM4R 0oM4R mentioned this pull request Oct 10, 2024
Closed
1 task
@samaradel samaradel requested a review from 0oM4R October 13, 2024 11:09
@samaradel samaradel marked this pull request as draft October 15, 2024 10:06
@samaradel samaradel marked this pull request as ready for review October 16, 2024 14:57
@0oM4R
Copy link
Contributor

0oM4R commented Oct 17, 2024

Screenshot from 2024-10-17 16-05-18
this field should got validated on submit i think

Screencast.from.2024-10-17.16-10-24.webm

@samaradel
Copy link
Contributor Author

@0oM4R ok, open an issue for this please

@0oM4R 0oM4R mentioned this pull request Oct 21, 2024
Open
@samaradel samaradel merged commit e87174a into development Oct 21, 2024
10 checks passed
@samaradel samaradel deleted the development_smtp_validator branch October 21, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0oM4R 0oM4R 0oM4R approved these changes

@amiraabouhadid amiraabouhadid amiraabouhadid approved these changes

@AhmedHanafy725 AhmedHanafy725 Awaiting requested review from AhmedHanafy725 AhmedHanafy725 is a code owner

@MohamedElmdary MohamedElmdary Awaiting requested review from MohamedElmdary MohamedElmdary is a code owner

@Mahmoud-Emad Mahmoud-Emad Awaiting requested review from Mahmoud-Emad Mahmoud-Emad is a code owner

@AlaaElattar AlaaElattar Awaiting requested review from AlaaElattar AlaaElattar is a code owner

@mohamedamer453 mohamedamer453 Awaiting requested review from mohamedamer453

@zaelgohary zaelgohary Awaiting requested review from zaelgohary zaelgohary is a code owner

@maayarosama maayarosama Awaiting requested review from maayarosama maayarosama is a code owner

@xmonader xmonader Awaiting requested review from xmonader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@samaradel @0oM4R @xmonader @zaelgohary @amiraabouhadid @maayarosama